Skip to content

Conversation

stunes-ms
Copy link
Contributor

@stunes-ms stunes-ms commented Sep 18, 2025

This change adds telemetry (in the form of event markers in our logs) around operations of interest for VMGS provisioning and certain vTPM features (AKPub, AKCert, etc.). The goal of this approach is to have structured, easily queryable data that can show that provisioning is successful and indicate whether Trusted Launch features (OpenHCL-provisioned VMGS, GSP key) are being enabled.

This PR is intended to match a corresponding legacy HCL change as closely as possible, given the differences in their logging implementations. In particular, the operation names (here, added as an op_type property on traces) and other metadata properties should match.

@stunes-ms stunes-ms requested a review from a team as a code owner September 18, 2025 23:18
@Copilot Copilot AI review requested due to automatic review settings September 18, 2025 23:18
@stunes-ms stunes-ms requested a review from a team as a code owner September 18, 2025 23:18
@stunes-ms stunes-ms requested review from tjones60 and removed request for Copilot September 18, 2025 23:18
@stunes-ms stunes-ms force-pushed the user/mikestunes/telemetry branch from 20afa23 to 44bfa47 Compare September 19, 2025 21:29
@Copilot Copilot AI review requested due to automatic review settings September 19, 2025 21:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive telemetry logging for VMGS provisioning and TPM operations in OpenHCL. The goal is to provide structured, queryable logs that can track provisioning success and indicate whether Trusted Launch features are being enabled.

Key changes include:

  • Added operation-specific telemetry logs with timing metrics across TPM and VMGS operations
  • Enhanced logging for AK cert provisioning, NV read/write operations, and GSP callbacks
  • Added BIOS GUID tracking to TPM device resources for better telemetry correlation

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
vm/vmgs/vmgs/src/vmgs_impl.rs Changed log level and added op_type for VMGS provisioning telemetry
vm/devices/tpm_resources/src/lib.rs Added bios_guid field to TpmDeviceHandle for logging purposes
vm/devices/tpm/src/tpm_helper.rs Added comprehensive telemetry for TPM NV read/write operations with timing
vm/devices/tpm/src/lib.rs Added detailed AK cert and key provisioning telemetry with error handling
vm/devices/get/guest_emulation_transport/src/client.rs Added GSP callback operation telemetry
openhcl/underhill_attestation/src/lib.rs Added VMGS decryption telemetry with GSP type tracking
Various config files Updated to pass bios_guid parameter and added dependencies

mingweishih
mingweishih previously approved these changes Sep 24, 2025
Copy link
Contributor

@mingweishih mingweishih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stunes-ms stunes-ms changed the title RFC: Add OpenHCL telemetry for VMGS provisioning Add OpenHCL telemetry for VMGS provisioning Sep 25, 2025
@stunes-ms stunes-ms force-pushed the user/mikestunes/telemetry branch from b39f77b to e66b622 Compare October 1, 2025 19:51
smalis-msft
smalis-msft previously approved these changes Oct 1, 2025
@mebersol
Copy link
Collaborator

mebersol commented Oct 3, 2025

    // If no DEK, then ingress was Gsp By Id (derived above)

is this logic correct in your change?


Refers to: openhcl/underhill_attestation/src/lib.rs:1002 in e66b622. [](commit_id = e66b622, deletion_comment = False)

Copy link

github-actions bot commented Oct 3, 2025

mebersol
mebersol previously approved these changes Oct 4, 2025
Copy link
Collaborator

@mebersol mebersol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link

tjones60
tjones60 previously approved these changes Oct 10, 2025
benhillis
benhillis previously approved these changes Oct 11, 2025
Copy link
Contributor

@smalis-msft smalis-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have this new telemetry crate, I don't think it's providing enough value, it combines separate concerns with its one enum, and it adds too much implicitness.

@stunes-ms stunes-ms dismissed stale reviews from benhillis and tjones60 via 19066e5 October 14, 2025 18:13
@stunes-ms stunes-ms force-pushed the user/mikestunes/telemetry branch 2 times, most recently from 19066e5 to 7042938 Compare October 14, 2025 18:38
smalis-msft
smalis-msft previously approved these changes Oct 14, 2025
Copy link
Contributor

@smalis-msft smalis-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have this

@stunes-ms
Copy link
Contributor Author

Rebased and squashed

@smalis-msft
Copy link
Contributor

This'll need a manager to bypass the binary size gate, but I'm ok with it.

@mebersol mebersol force-pushed the user/mikestunes/telemetry branch from b68f908 to 812ba8b Compare October 17, 2025 00:35
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants